-
Notifications
You must be signed in to change notification settings - Fork 990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Perf: Kernel Sync Performance #1987
Conversation
…antioch and igno.
…ck_hash (to verify chain) and a first_kernel_index.
…ly requests synchronously from 1 peer.
…till need to only send Outputs & RangeProofs (no kernels) to peers that support ENHANCED_TXHASHSET_HIST.
chain/src/pipe.rs
Outdated
|
||
if ctx.txhashset.num_kernels() < (first_kernel_index + 1) { | ||
txhashset::extending(&mut ctx.txhashset, &mut ctx.batch, |extension| { | ||
// DAVID: Rewind mmr to correct kernel index just to be safe? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make sense.
chain/src/pipe.rs
Outdated
|
||
// DAVID: If kernel is last in block, validate root. | ||
// Use RewindableKernelView? Or modify validate_kernel_history? | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd consider pushing that whole loop down in txhashset in some apply_kernels
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also shouldn't there be some MMR reconstruction after this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, looks like apply does it. But there should be some clearer separation around validation. Here you're verifying each kernel, but the whole state validation will do that again. So we either slip the latter one or this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends what you mean by whole state validation. If you're talking about after the TxHashSet is fully downloaded and processed, sure. But then we'd have to go back and start the kernel validation process all over again if something's wrong with the kernel. If instead you're referring to the expected call to "validate_kernel_history" on line 309, then I disagree. It doesn't verify the signature of the kernel. It only verifies that the MMR root matches the header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After re-reading, it appears you're talking about verify_kernel_signatures in Extension. Yea, I can probably skip that verify if it's already been done. It'd probably be easiest to store a 'validated' boolean on the kernel. If you're opposed to adding the extra byte (since kernels are all written to disk), I can look into other options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the skipping of kernels zipping that's missing, looking good overall.
chain/src/pipe.rs
Outdated
|
||
// DAVID: If kernel is last in block, validate root. | ||
// Use RewindableKernelView? Or modify validate_kernel_history? | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also shouldn't there be some MMR reconstruction after this?
chain/src/pipe.rs
Outdated
|
||
// DAVID: If kernel is last in block, validate root. | ||
// Use RewindableKernelView? Or modify validate_kernel_history? | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, looks like apply does it. But there should be some clearer separation around validation. Here you're verifying each kernel, but the whole state validation will do that again. So we either slip the latter one or this one.
previous approaches have included (BTC example): verify twice, followed by
changing one to an assertation that is optimized out for production
builds, followed
by removing the really working check. From this, maybe keeping a bool is
wise. The bool can be stripped before serialization to disk.
|
I'm not sure we can strip the bool before serialization to disk. We'd be flushing to disk shortly after first validation I believe, and wouldn't perform the second validation (where boolean is needed) until much later in the sync process when we receive the outputs in rangeproofs as part of TxHashSet download. |
…eight indices haven't been built yet. Using header mmr instead.
@antiochp Requested that this be reviewed and merged in several phases. Closing this one and opening one for the first phase. |
Sync Performance: Support requesting kernel MMRs separate from the rest of TxHashSet
#1968